Follow-ups to #660 by tankyleo · Pull Request #992 · lightningdevkit/ldk-node

tankyleo · 2026-07-17T05:18:04Z

Fixes #989

ldk-reviews-bot · 2026-07-17T05:18:06Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tankyleo · 2026-07-17T05:19:53Z

 use crate::Error;

-const BCAST_PACKAGE_QUEUE_SIZE: usize = 50;
+const BCAST_PACKAGE_QUEUE_SIZE: usize = 256;


I went back and forth with codex on this one, for now I lean against drawing transactions at random order: we do not do this now, but in the future we might want to make sure we broadcast parents before children no ?

but in the future we might want to make sure we broadcast parents before children no ?

Well, in short, I'm not sure we could even begin to guarantee this? Broadcast is inherently fallible, we never know when the network connection could drop, when the backend won't accept anything into the mempool, and when it will just decide to drop any transaction again. So without mempool introspection I'd always lean on treating broadcast as an entirely opaque operation: we submit and retry, and only stop once we see what we expect confirmed in a block.

sounds good added the random draw below

tankyleo · 2026-07-17T05:21:32Z

 }

+fn esplora_submitpackage_error_implies_unsupported(e: &esplora_client::Error) -> bool {
+	matches!(e, esplora_client::Error::HttpResponse { status: 400 | 404, .. })


We map 400 here to ChainSourceNotSupported because Bitcoin Core v26 returns a RPC error when submitting the dummy package, which maps to error code 400.

Grrr, this is all very brittle. I can't wait to drop all of this logic again once we can just assume submitpackage is available if we have any chain source at all.

tnull · 2026-07-17T08:10:52Z

-		})?;
+		});
+		if let Err(e) = startup_chain_check_res {
+			self.chain_source.stop();


Actually, it seems there are a bunch of (pre-existing) potential cases below where we'd error. To solve this once and for all, let's rename start to start_inner and add a new pub fn start that wraps that start_inner (probably also takes the running lock) and stops the chain source for any error returned.

tnull · 2026-07-17T08:12:31Z

      - name: Test with 0FC enabled
        run: |
          RUSTFLAGS="--cfg no_download --cfg cycle_tests --cfg tokio_unstable --cfg zero_fee_commitment_tests" cargo test -- --test-threads=1
+  eclair-interop-test:


Hmm, any chance we could make this an interop test for 0fc channels, not a 0fc test that does interop? I.e., can this be (a cfg-gated) part of the regular eclair tests, where we already have the docker setup etc.?

tnull

This needs a rebase post #996.

Btw, it seems the 0fc integration test is flaky, i.e., timing out in CI very often. Would be good to investigate why and include a fix as part of this PR.

tnull · 2026-07-21T08:23:58Z

+		match self.start_inner(&mut is_running_lock) {
+			Ok(()) => Ok(()),
+			Err(e) => {
+				self.chain_source.stop();


Codex:

[P2] Roll back background tasks after startup failure. /home/tnull/worktrees/ldk-node/pr-992-latest-20260721/src/lib.rs:294 now stops only the chain source when start_inner fails. However, wallet sync, RGS, and pathfinding tasks are spawned before listener resolution/binding can fail at
lines 427–473. The node remains “not running,” so stop() cannot clean them up, while another start() creates duplicate loops. Move fallible listener setup before task spawning or perform a complete task rollback.

In 2024749, we started taking one slot in the package queue for each transaction broadcasted by the wallet upon `WalletEvent::ChainTipChanged`, so we increase the number of slots available in the queue.

tnull

Thanks! Seems the 0fc integration tests are still timing out in CI here?

tnull · 2026-07-24T10:52:32Z

 								})
 								.collect()
 						};
+						for i in (1..txs_to_broadcast.len()).rev() {


Hmm, shouldn't this be a feature of the broadcaster queue in general rather than just randomizing this one callsite? Maybe we could just have it drop random entries when the queue is full?

The broadcast queue itself doesn't currently allow us to drop random entries when it is full; this would require a new data structure inside the queue that allows for this random removal.

If we want this, I think this should be a separate PR no?

Hmm, fair. This PR seems to keep growing in scope. Let's just revert to only bumping the number of entries here and we can revisit once we think we have actual evidence it's an issue.

tnull · 2026-07-24T10:58:31Z

 /// We use this parent-child TRUC package to make sure the configured chain source supports
 /// broadcasting packages via the `submitpackage` Bitcoin Core RPC.
-const PARENT_TXID: &str = "9a015f93fac6cb203c2b994e18b85176eb0354a22a468255516f3c6002d3f696";
+const DUMMY_PACKAGE_EXPECTED_ERROR: &str = "bad-txns-inputs-missingorspent";


Hmm, matching on the exact string seems not very robust. Seems some minor change in how Bitcoin Core handles errors could lead to LDK Node not starting anymore.

As mentioned on #660 (comment), maybe we just need to accept that we currently don't have a good way of checking all backends support v29+? Should we drop this commit?

I could totally see us dropping this commit. Here's my argument against dropping it:

We'd only abort startup in case enable_zero_fee_commitments is true, and we default to false at the moment. So given this default, I think we can be more aggressive in how we check for 0FC support once the flag is turned on. If someone turns this flag on and fails to start, they can easily turn this flag back off.

With codex, I've tested this validation across 26, 27, 28, 29, 30, and 31, and across mempool, blockstream, romanz, Fulcrum, and ElectrumX. With codex I think we can easily test this logic against upcoming releases as they come out, since Core only ships two releases a year, and we can adjust this as the reality on the ground changes.

We'd only abort startup in case enable_zero_fee_commitments is true, and we default to false at the moment. So given this default, I think we can be more aggressive in how we check for 0FC support once the flag is turned on. If someone turns this flag on and fails to start, they can easily turn this flag back off.

Hmm? What happens if they enable it, then open 0fc channels, then the backend changes something subtly and they fail to startup. Also note that the user might not be an operator that is able to debug this, it might simply be that the backend changes something and this results in their mobile app crashing as we fail to startup.

Thanks I previously did not give much weight to this possibility, but now I see how this could happen, I dropped the commit below.

tnull · 2026-07-24T10:59:38Z

      - name: Test with 0FC enabled
        run: |
-          RUSTFLAGS="--cfg no_download --cfg cycle_tests --cfg tokio_unstable --cfg zero_fee_commitment_tests" cargo test -- --test-threads=1
+          RUSTFLAGS="--cfg no_download --cfg cycle_tests --cfg tokio_unstable --cfg zero_fee_commitment_tests" cargo test


Seems that should be a fixup commit?

I was just testing things out to see if removing this flag helped. It doesn't look like it I am going to revisit this.

tnull · 2026-07-24T11:05:27Z

 			return Err(Error::AlreadyRunning);
 		}

+		match self.start_inner(&mut is_running_lock) {


Codex:

[P1] Fully unwind late startup failures — /home/tnull/worktrees/ldk-node/pr-992-latest-20260724/src/lib.rs:295

A listener resolution/bind failure can occur after wallet sync and other background tasks have already spawned. The new error handler only stops the chain source. Since is_running remains false, callers cannot invoke stop(), and retrying start() leaves duplicate tasks running. Fallible
listener setup should occur before spawning tasks, or the error path must fully unwind them.

Seems that might be worth an additional PR though. Let me know if you prefer I pick that up.

Let me know what you think of the commit below, we move the fallible operations before starting the tasks.

I don't think we'd want to do this, as we deliberately allow inbound connections as one of the last steps after we're positive we're ready-to-go.

Help me understand thank you, once we've passed all the can-fail calls, aren't we positive we are ready to go ?

tnull · 2026-07-24T11:06:06Z

+}
+
+#[uniffi::export]
+impl ChannelTypeFeatures {


Codex:

[P2] Expose all channel-type feature flags to bindings — /home/tnull/worktrees/ldk-node/pr-992-latest-20260724/src/ffi/types.rs:1830

ChannelTypeFeatures omits typed accessors for option_scid_alias and option_zeroconf, although both are valid ChannelTypeContext features supported by the underlying type. Rust callers retain those methods, but UniFFI users must manually decode to_bytes().

Done below.

tnull

Feel free to squash fixups (or drop as discussed above).

tnull · 2026-07-27T09:25:24Z

 jobs:
  build-and-test:
-    timeout-minutes: 60
+    timeout-minutes: 75


Hmm, if this is the only issue, let's bump the timeout to to hours across the board to avoid other workflows hitting the same issue?

This was not the issue, I reverted this, and instead I now fetch feerates and validate zero-fee commitments support serially, see the commit below.

tnull · 2026-07-27T09:44:33Z

 /// We use this parent-child TRUC package to make sure the configured chain source supports
 /// broadcasting packages via the `submitpackage` Bitcoin Core RPC.
-const PARENT_TXID: &str = "9a015f93fac6cb203c2b994e18b85176eb0354a22a468255516f3c6002d3f696";
+const DUMMY_PACKAGE_EXPECTED_ERROR: &str = "bad-txns-inputs-missingorspent";


We'd only abort startup in case enable_zero_fee_commitments is true, and we default to false at the moment. So given this default, I think we can be more aggressive in how we check for 0FC support once the flag is turned on. If someone turns this flag on and fails to start, they can easily turn this flag back off.

Hmm? What happens if they enable it, then open 0fc channels, then the backend changes something subtly and they fail to startup. Also note that the user might not be an operator that is able to debug this, it might simply be that the backend changes something and this results in their mobile app crashing as we fail to startup.

tnull · 2026-07-27T09:45:25Z

 			return Err(Error::AlreadyRunning);
 		}

+		match self.start_inner(&mut is_running_lock) {


I don't think we'd want to do this, as we deliberately allow inbound connections as one of the last steps after we're positive we're ready-to-go.

tnull · 2026-07-27T09:48:27Z

 								})
 								.collect()
 						};
+						for i in (1..txs_to_broadcast.len()).rev() {


Hmm, fair. This PR seems to keep growing in scope. Let's just revert to only bumping the number of entries here and we can revisit once we think we have actual evidence it's an issue.

This is particularly relevant for the electrum chain source; if we fail to fetch feerates, or zero fee commitments validation fails, and we do not stop the electrum chain source before returning an error, then the user will hit a debug assertion on the next restart.

Co-Authored-By: HAL 9000

Resolve and bind configured listening addresses before starting wallet sync, gossip sync, pathfinding score sync, and the remaining background loops. If listener setup now fails, startup returns while only the chain source needs cleanup, so a retry cannot leave duplicate loops behind. AI-assisted-by: OpenAI Codex

Add a UniFFI wrapper so bindings can inspect channel type flags. Update anchor accounting tests to use channel type features instead of inferring zero-fee commitments from the commitment feerate. Co-Authored-By: HAL 9000

Co-Authored-By: HAL 9000

In case of a timeout, these logs are very useful, and would otherwise fail to show.

When concurrent requests share an Electrum client connection, a clean EOF can cause the active reader to return an error without waking other requests waiting on the connection. Those requests remain blocked in spawn_blocking, causing Tokio runtime shutdown to wait indefinitely for them to finish. As a temporary fix, we run the startup fee update and zero-fee commitment validation in sequence. This avoids concurrent requests on the shared Electrum connection leaving a blocking task stranded after connection closure. Co-Authored-By: HAL 9000

tankyleo · 2026-07-28T05:09:44Z

The third run just passed, I believe CI for 0FC is now stable, will launch another run to check things again.

tankyleo requested a review from tnull July 17, 2026 05:18

tankyleo commented Jul 17, 2026

View reviewed changes

tnull reviewed Jul 17, 2026

View reviewed changes

tankyleo force-pushed the 2026-07-0fc-followups branch 2 times, most recently from edf8512 to bda78cc Compare July 17, 2026 17:46

tankyleo requested a review from tnull July 17, 2026 18:38

tnull reviewed Jul 21, 2026

View reviewed changes

Increase the size of the broadcast package queue

4e7516b

In 2024749, we started taking one slot in the package queue for each transaction broadcasted by the wallet upon `WalletEvent::ChainTipChanged`, so we increase the number of slots available in the queue.

tankyleo force-pushed the 2026-07-0fc-followups branch from bda78cc to 30616f5 Compare July 24, 2026 04:53

tnull added this to the 0.8 milestone Jul 24, 2026

tnull reviewed Jul 24, 2026

View reviewed changes

tankyleo force-pushed the 2026-07-0fc-followups branch 3 times, most recently from 114d9c9 to 35d0a28 Compare July 24, 2026 23:52

tnull reviewed Jul 27, 2026

View reviewed changes

tnull mentioned this pull request Jul 27, 2026

Electrum chain source stop() discards registered Filter entries #1005

Open

tankyleo force-pushed the 2026-07-0fc-followups branch from dd69242 to f7608d5 Compare July 27, 2026 21:17

tankyleo added 7 commits July 28, 2026 01:08

f: Stop chain source after *all* startup failures

4695444

Co-Authored-By: HAL 9000

Expose ChannelTypeFeatures in ChannelDetails

5bd0bf1

Add a UniFFI wrapper so bindings can inspect channel type flags. Update anchor accounting tests to use channel type features instead of inferring zero-fee commitments from the commitment feerate. Co-Authored-By: HAL 9000

Add Eclair interop test for zero-fee-commitments

aade5c2

Co-Authored-By: HAL 9000

Do not capture logs in 0FC channels CI

2da79df

In case of a timeout, these logs are very useful, and would otherwise fail to show.

tankyleo force-pushed the 2026-07-0fc-followups branch from f7608d5 to 3136a05 Compare July 28, 2026 01:52

tankyleo requested a review from tnull July 28, 2026 05:09

Uh oh!

Conversation

tankyleo commented Jul 17, 2026 • edited by tnull Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tankyleo Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tankyleo commented Jul 17, 2026 •

edited by tnull

Loading

ldk-reviews-bot commented Jul 17, 2026 •

edited

Loading

tankyleo Jul 17, 2026 •

edited

Loading

tankyleo Jul 17, 2026 •

edited

Loading

tankyleo Jul 24, 2026 •

edited

Loading

tankyleo Jul 28, 2026 •

edited

Loading